Skip to content

Conversation

@halspang
Copy link
Member

When an orchestration was being rejected due to a versioning mismatch an issue would cause the history events of the orchestration to build continuously. This was caused by the orchestration adding an OrchestrationStarted event before the versioning took place and was saved even on a rejected version.

This change causes the versioning code to exit the process method entirely instead of just prematurely end the process loop. This avoids the commit of the history.

@halspang halspang requested review from AnatoliB and cgillum May 22, 2025 21:11
@AnatoliB
Copy link
Collaborator

Do you think we have sufficient test coverage for this? Wondering if it would be possible (and practical) to add a test that fails without this change.

When an orchestration was being rejected due to a versioning mismatch
an issue would cause the history events of the orchestration to build
continuously. This was caused by the orchestration adding an
OrchestrationStarted event before the versioning took place and was
saved even on a rejected version.

This change causes the versioning code to exit the process method
entirely instead of just prematurely end the process loop. This avoids
the commit of the history.

Signed-off-by: halspang <[email protected]>
@halspang halspang force-pushed the halspang/fix_reject_history branch from e550739 to e587e35 Compare May 23, 2025 00:02
@halspang
Copy link
Member Author

@AnatoliB - Updated a test that looks at this scenario. I confirmed manually that it passes with this change and fails without it.

Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fix is 90% correct. See my one request below about changing the false value to true. Then I'll be ready to sign off.

@halspang halspang requested a review from cgillum May 23, 2025 21:25
@cgillum cgillum merged commit 6674c79 into Azure:main May 23, 2025
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants